Skip to content

Fix: data-disabled CSS selector for DateField #7896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

suyash5053
Copy link
Contributor

Closes #7890

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suyash5053
Copy link
Contributor Author

@snowystinger Yeah Sure, I was waiting to see whether I'm on the right track or not. Will write the test cases :).

@suyash5053 suyash5053 requested a review from snowystinger March 10, 2025 00:45
@suyash5053
Copy link
Contributor Author

Hey @snowystinger, I have added some test cases, feel free to suggest me some more if needed.

A different question is, in TimeField we also see the disabled state but there are no test cases related to them. Is this an issue or we just left them knowingly?

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests, I left some comments

@suyash5053 suyash5053 requested a review from snowystinger March 10, 2025 18:21
@suyash5053
Copy link
Contributor Author

Please direct me with a specific test case and approach that I should have, if I'm not understanding something. @snowystinger

@Persists
Copy link
Contributor

Persists commented Mar 11, 2025

I’d like to add a few considerations, as this PR caught my attention. Currently, for DateField and TimeField I don't see the necessity to keep data props bth in the DateInput and the Corresponding Field component. What is the purpose of this?

In addition, data props such as data-required and data-readonly may need to be added too.

@suyash5053 suyash5053 requested a review from snowystinger March 11, 2025 20:03
@yihuiliao
Copy link
Member

@Persists For data-required and data-readonly, there is already an open issue for that: #6151. I think that can be handled separately in a different PR.

@snowystinger
Copy link
Member

@Persists is this making it difficult to implement something?

Currently, for DateField and TimeField I don't see the necessity to keep data props both in the DateInput and the Corresponding Field component. What is the purpose of this?

Otherwise, it can be useful, particularly when using utility based CSS, to not make selectors that reference parents.

@Persists
Copy link
Contributor

@yihuiliao Thank you!

@snowystinger Thanks! That makes sense.

@reidbarber reidbarber added this pull request to the merge queue Mar 13, 2025
Merged via the queue into adobe:main with commit 6835cce Mar 13, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

data-disabled CSS selector not added to DateField
5 participants